Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enhancements and refactoring: Ruff checks resolved #443

Conversation

MilagrosMarin
Copy link
Contributor

@MilagrosMarin MilagrosMarin commented Oct 29, 2024

This PR closes #441 and #417 by removing the exclusion of ruff checks for dj_pipeline from pyproject.toml.

  • UPDATE: I’ve added comments in this PR to highlight code changes and make the review process easier for reviewers.
  • UPDATE2: the following changes have been reverted:
    • Revert black formatting ( revert max line length to 88 instead of 105)
    • Revert changes in streams.py
    • Revert changes about Assertion replacement in tests

This is a summary of the relevant changes that have been implemented, with all checks passing:

Resolved issues for aeon/dj_pipeline:

  • B006: Fixed mutable default argument in plotting.py.
  • B021: Removed dynamic references and made the docstring static in DeviceDataStream.key_source, ensuring it remains informative.
  • D101, D102, D103, D105, D106: Added new docstrings.
  • E501: Corrected most lines that were too long, adding exceptions in a few cases to preserve the intended meaning.
  • D202, D205: Improved docstring formatting.
  • F401, F403: Removed unused imported dependencies.
  • I001: Sorted dependencies.
  • B905: Added strict=False to the zip function to prevent the function from raising a ValueError if the iterables passed to it do not have the same length.
  • Security: Replaced hashlib.md5 with hashlib.sha256 due to known vulnerabilities in the MD5 hashing algorithm -> Reverted: Change was reverted, and an ignore rule was added due to potential breakage introduced by the update.
  • B904: Enhanced exception handling by using as err and raising exceptions with raise ... from err to differentiate errors.
  • UP038: Used the new X | Y syntax for isinstance calls, as introduced in Python 3.10.
  • PLR2004: Replaced magic values with constant variables.
  • SIM108: Refactored visit_analysis to use the ternary operator.
  • Removed checks from pyproject.toml list: No issues found for B006, B021, S110, UP038, E999, E722, F821, S324, S605, S607, E999, PLR2004.
  • README.md: Added some text.
  • Refactor: Updated the read method in the JsonList class within aeon/io/reader.py.
  • E741: Renamed the variables in gen_hex_grad in aeon/analysis/block_plotting.py.
  • Refactor: Made improvements to dictionary handling in aeon/analysis/block_plotting.py.
  • # noqa Exceptions: Added # noqa comments for specific cases, e.g., in reader for F821 and # noqa PLW0127 for visit_analysis.py.

Additional fixes to the rest of the reporitory:

In addition to the above changes, I have also resolved most of the ruff lint issues from the ignored list in pyproject.toml, ensuring all checks passed:

  • D100, D104, D105, D107: Added missing docstrings.
  • E201, E202, E203, E231, E702: Corrected whitespace and formatting.
  • S101: Replaced assertions with exceptions to ensure expected behavior in all scenarios; also added logger=dj.logger where appropriate. -> Reverted assertions replacement in tests and added rule in pyproject since it is acceptable to use for small, simple tests.
  • PT013: Fixed the import of pytest in tests.
  • pyproject.toml updated accordingly
  • All checks passed after the latest changes in this PR.

Deprecation Fix:

  • Updated all instances of datetime.utcnow() to datetime.now(timezone.utc) to address deprecation issues.

ttngu207 and others added 30 commits August 8, 2024 08:52
…Centre/gl-issue-418

Allow reading pose model metadata from local folder
why? corrupted data in harp files? not sure
@MilagrosMarin
Copy link
Contributor Author

@lochhh and @ttngu207 , thank you both so much for your reviews! I’ve completed the third round of review for this PR. Since there were many new suggestions in different forms, could you please @lochhh give it a final review? Thank you!

Copy link
Contributor

@lochhh lochhh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @MilagrosMarin for addressing all comments. I made another PR here with some final suggestions. And just one last comment about noqa B023 here:

df[column] = df[self.root_key].apply(lambda x: x[column]) # noqa B023

If it's too much trouble to test this we can also keep the current change.

df = pd.read_json(f, lines=True)
df.set_index("seconds", inplace=True)
for column in self.columns:
df[column] = df[self.root_key].apply(lambda x: x[column])
df[column] = df[self.root_key].apply(lambda x: x[column]) # noqa B023
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MilagrosMarin has this been tested?

@ttngu207
Copy link
Contributor

Thanks @MilagrosMarin for addressing all comments. I made another PR here with some final suggestions. And just one last comment about noqa B023 here:

df[column] = df[self.root_key].apply(lambda x: x[column]) # noqa B023

If it's too much trouble to test this we can also keep the current change.

I have tested this, good to go here

@ttngu207 ttngu207 merged commit 9be1f8e into SainsburyWellcomeCentre:datajoint_pipeline Nov 20, 2024
@ttngu207
Copy link
Contributor

All comments/suggestions have been addressed/resolved
PR merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants